Add quantile mapping and associated tests #2264
Conversation
maxwhitemet
force-pushed
the
mobt_1007_quantile_mapping_plugin
branch
from
1c47068 to
73363ed
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2264 +/- ##
==========================================
- Coverage 98.39% 95.19% -3.20%
==========================================
Files 124 150 +26
Lines 12212 15323 +3111
==========================================
+ Hits 12016 14587 +2571
- Misses 196 736 +540 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
maxwhitemet
force-pushed
the
mobt_1007_quantile_mapping_plugin
branch
from
73363ed to
ae2a5ad
Compare
gavinevans
left a comment
gavinevans
left a comment
There was a problem hiding this comment.
Thanks @maxwhitemet 👍
I've added some comments below.
- Move functionality into QuantileMapping class - Remove redundancy - Increase variable name clarity - Refactor into smaller functions 2. Additions: - Improved readability experience of docstrings - Fixed improper masked array handling
maxwhitemet
force-pushed
the
mobt_1007_quantile_mapping_plugin
branch
from
ae2a5ad to
6ec215f
Compare
There was a problem hiding this comment.
In addition to the feedback received, I have implemented the below modifications:
- Made lots of changes to docstrings, such that now:
- More extensive documentation has moved from private to public methods
- Removed redundant Args sections in private methods, defined elsewhere.
- Masked arrays
- I was concerned about what would happen if the reference cube and the post-processed forecast cube had differing mask locations. Thus I have added handling that may require further discussion: combine the masks such that only points that are valid in both cubes are used to build the CDFs.
- Removed redundant use of np.where for non-masked arrays as I discovered this is implicitly handled in np.ma.where
gavinevans
left a comment
gavinevans
left a comment
There was a problem hiding this comment.
Thanks for the simplications @maxwhitemet 👍
I've added a few minor comments.
| [ | ||
| 6.8, | ||
| 7.7, | ||
| 8.7, |
There was a problem hiding this comment.
I think if you removed this comma, these values would no longer spread across multiple lines.
| 8.7, | |
| 8.7 |
| output_cube.data = np.ma.masked_array( | ||
| corrected_data_reshaped, mask=output_mask | ||
| ) |
There was a problem hiding this comment.
The interesting thing about this, is that it means that the mask on the output cube could be different to that on the input forecast. In practice, I'm not expecting this situation, but it might be worth noting this in the docstring as this might not be expected.
| For each forecast value: | ||
| 1. Find its quantile position in the forecast distribution | ||
| 2. Map that quantile to the corresponding value in the reference distribution | ||
| using discrete (floor) method | ||
|
|
||
| Example: | ||
| - reference_data: [10, 20, 30, 40, 50] | ||
| - forecast_data: [5, 15, 25, 35, 45] | ||
|
|
||
| The forecast systematically underestimates by 5 units. | ||
| Corrected values: [10, 20, 30, 40, 50] (mapped to reference distribution) |
There was a problem hiding this comment.
The formatting in this docstring is currently generated a Sphinx error: https://github.com/metoppv/improver/actions/runs/20619700743/job/59219100476?pr=2264
2 participants
Addresses #1007
This PR implements quantile mapping into the IMPROVER repo, adding a quantile mapping module, CLI, unit tests, and acceptance tests.
A demonstration of the plugin's functionality is available here.
Testing: